Fix against se-0184a #1247

tayloraswift · 2017-10-02T23:04:19Z

ianpartridge · 2017-10-03T09:51:40Z

Foundation/Host.swift

@@ -38,8 +38,8 @@ open class Host: NSObject {
    static internal func currentHostName() -> String {
        let hname = UnsafeMutablePointer<Int8>.allocate(capacity: Int(NI_MAXHOST))
        defer {
-            hname.deinitialize()
-            hname.deallocate(capacity: Int(NI_MAXHOST))
+            hname.deinitialize(count: 1)


I've always found this method confusing. What does it actually do?

https://developer.apple.com/documentation/swift/unsafemutablepointer/1641720-deinitialize is no help, it just says that deinitialize() deinitializes! Does it zero the memory, for example? I assume not otherwise the documentation would say so.

If it is just changing the "state" of the memory then I question the meaning/utility of the "states".

@ianpartridge Isn't deinitialize for objects where the deinit method is called on them? If Im correct its not needed for arrays of Int8

No I think this is unrelated to deinit which only applies to classes. UnsafeMutablePointer et al are structs.

I was referring to the type stored in the allocated memory not the UnsafeMutablePointer itself.

Oh I see what you mean, sorry :) If that's correct then the doc doesn't mention it!

deinitialize does nothing for trivial types, it doesn’t actually need to be there

Oh OK! Could you update this PR to remove it then?

I’ve removed those calls in Host.swift though I would hold off removing them elsewhere because it could inadvertently introduce memory bugs if the type isn’t absolutely a trivial type and will never be anything but a trivial type in the future

ianpartridge · 2017-10-03T09:53:34Z

Foundation/NSConcreteValue.swift

-        self._storage = UnsafeMutableRawPointer.allocate(bytes: self._typeInfo.size, alignedTo: 1)
-        self._storage.copyBytes(from: value, count: self._typeInfo.size)
+        self._storage = UnsafeMutableRawPointer.allocate(byteCount: self._typeInfo.size, alignment: 1)
+        self._storage.copyMemory(from: value, byteCount: self._typeInfo.size)


This one doesn't read as an improvement to me - the previous API seemed quite clear as to what it would do. Maybe you are fixing some overloading of the meaning of count: though? I'm not sure.

this was discussed heavily during the review of SE-0184. The core team felt strongly that it should be copyMemory and byteCount.

ianpartridge

Could you link me to SE-0184a - I couldn't find it?

Please take my comments as a non-expert's comment to the proposal not this PR per se. Hopefully seeing how someone who has not been immersed in the discussions reacts when confronted with the changes is interesting.

tayloraswift · 2017-10-03T18:11:20Z

SE 184
SE 184a

alblue · 2017-10-10T10:13:49Z

@swift-ci please test

tayloraswift · 2017-10-10T20:27:04Z

you have to test against the swift PR

alblue · 2017-10-11T19:03:34Z

@swift-ci please test

alblue · 2017-10-13T08:40:56Z

@swift-ci please test

alblue · 2017-10-13T09:04:07Z

The errors in the build are:

Foundation/AffineTransform.swift:317:55: error: incorrect argument labels in call (have 'byteCount:alignment:', expected 'bytes:alignedTo:')
        let pointer = UnsafeMutableRawPointer.allocate(byteCount: MemoryLayout<Float>.stride * 6, alignment: 1)
                                                      ^~~~~~~~~~                                  ~~~~~~~~~
                                                       bytes                                      alignedTo

tayloraswift · 2017-10-14T01:38:49Z

@alblue This PR will only build if you build it with the actual compiler modifications in @12200

alblue · 2017-10-16T11:04:04Z

OK, so when swiftlang/swift#12200 is merged then we can revisit this request

ianpartridge · 2017-10-16T11:26:22Z

Although SE-184 has been accepted, this PR seems to be for SE-184a - I'm not sure how that relates?

@Kelvin13 will SE-184a require a separate Swift Evolution review cycle? Is this PR dependent on another Swift Evolution proposal beyond SE-184?

If so, I don't think we should keep this PR open for potentially months while that process proceeds. We should close it and reopen it when it's ready. The evolution proposal can link to the closed PR in the meantime.

Thoughts?

tayloraswift · 2017-10-17T00:00:17Z

@ianpartridge SE-184a is SE-184, SE-184b will likely get a completely different Swift evolution number. It’s confusing I know. Originally it was all one proposal, it got split up in review.

ianpartridge · 2017-10-17T08:35:30Z

Oh great, thanks for clarifying! In that case, as SE-184 is Accepted we should keep this open.

Do you have an estimate of when you hope to land the implementation of SE-184 into master?

tayloraswift · 2017-10-17T17:20:33Z

ask @atrick it could be as early as today if we can get the @availableity sorted out

ianpartridge · 2017-10-18T08:45:35Z

Great. By the way, this needs rebasing as it has a conflict now.

tayloraswift · 2017-11-06T21:13:51Z

done

alblue · 2017-11-10T11:46:28Z

@Kelvin13 has SE-184 landed in Swift yet, and if so, can this be merged? If not, do you have a link for the PR in swift that is blocking this PR?

atrick · 2017-11-18T00:42:48Z

This is being blocked by swiftlang/swift#12200

This reverts commit 1b12e83, reversing changes made to c13b850.

Revert "Merge pull request #1247 from kelvin13/se-0184a"

jckarter mentioned this pull request Oct 2, 2017

SE 0184a implementation swiftlang/swift#12200

Merged

ianpartridge reviewed Oct 3, 2017

View reviewed changes

swiftlang deleted a comment from tayloraswift Oct 10, 2017

compatability with SE-0184 implementation

6476c4f

atrick merged commit 1b12e83 into swiftlang:master Nov 18, 2017

slavapestov added a commit to slavapestov/swift-corelibs-foundation that referenced this pull request Nov 18, 2017

Revert "Merge pull request swiftlang#1247 from kelvin13/se-0184a"

62fcab1

This reverts commit 1b12e83, reversing changes made to c13b850.

slavapestov added a commit that referenced this pull request Nov 18, 2017

Merge pull request #1327 from slavapestov/revert-pointer-change-4.1

b247e71

Revert "Merge pull request #1247 from kelvin13/se-0184a"

parkera added a commit that referenced this pull request Nov 28, 2017

Revert "Revert "Merge pull request #1247 from kelvin13/se-0184a""

dfa8350

Fix against se-0184a #1247

Fix against se-0184a #1247

Uh oh!

Conversation

tayloraswift commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge left a comment

Choose a reason for hiding this comment

Uh oh!

tayloraswift commented Oct 3, 2017

Uh oh!

alblue commented Oct 10, 2017

Uh oh!

tayloraswift commented Oct 10, 2017

Uh oh!

alblue commented Oct 11, 2017

Uh oh!

alblue commented Oct 13, 2017

Uh oh!

alblue commented Oct 13, 2017

Uh oh!

tayloraswift commented Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alblue commented Oct 16, 2017

Uh oh!

ianpartridge commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tayloraswift commented Oct 17, 2017

Uh oh!

ianpartridge commented Oct 17, 2017

Uh oh!

tayloraswift commented Oct 17, 2017

Uh oh!

ianpartridge commented Oct 18, 2017

Uh oh!

tayloraswift commented Nov 6, 2017

Uh oh!

alblue commented Nov 10, 2017

Uh oh!

atrick commented Nov 18, 2017

Uh oh!

Uh oh!

ianpartridge Oct 3, 2017 •

edited

Loading

tayloraswift commented Oct 14, 2017 •

edited

Loading

ianpartridge commented Oct 16, 2017 •

edited

Loading